fix(engine): correct the inverted depth sign in 3D projection - #3232
fix(engine): correct the inverted depth sign in 3D projection#3232vanceingalls wants to merge 5 commits into
Conversation
Picks up crbug 522872457's fix (CL 8032671), which landed after the 152.0.7935.0 canary cut and so was absent from the old 152.0.7928.2 pin. Re-probed every 3D signal the compile gate matches, drawElementImage vs a CDP screenshot of the identical state, on the shipping headless-shell binary. PSNR, old pin -> new pin: backface-visibility:hidden 1.4 dB -> 14.8 dB still DAMAGED preserve-3d (no backface) 46.7 dB -> 46.7 dB clean perspective() 45.2 dB -> 45.2 dB clean matrix3d() 45.2 dB -> 45.2 dB clean rotate3d() 45.3 dB -> 45.3 dB clean translateZ under perspective 29.9 dB -> 29.9 dB marginal The upstream fix repaired the collateral damage only: dropped sibling content and lost backgrounds now render, but a culled backface is still painted. So the 3D gate stays. Beta rather than Canary because 153.0.8000.0 measured identical on every variant. Follow-up filed as PRINFRA-486: four of the five signals the gate matches were never broken on any build tested, so it may be able to narrow to backface-visibility alone. Needs a corpus eval first — this probe covers static angles only, and animated 3D subtrees take a different path.
`threeDProjection` runs a depth test — `gl.enable(gl.DEPTH_TEST)` with `clearDepth(1)` and `depthFunc(LEQUAL)` — but the comparison was inverted. CSS puts +z toward the viewer, GL treats larger ndc z as FARTHER, and the ndc matrix passed CSS z through with a POSITIVE scale. So the nearest quad received the largest depth, lost the test, and was occluded by the quad behind it. The y-flip on the row above changes handedness. That had already been compensated for winding (`frontFace(gl.CW)`) but never for depth, so it only surfaced once two quads in one context could be visible simultaneously — a single-quad context has nothing to lose a depth test against, which is why it went unnoticed. Negating the z row fixes it at source. Measured on two planes at ±45° in one preserve-3d context, drawElement vs a screenshot render of the same comp: 18.8 dB -> 51.2 dB (visually identical to the screenshot arm) Single-quad cases are unchanged: backface flip card at rest 54.5 dB, perspective+rotationX 51.1 dB, matrix3d 49.5 dB. Not unit-testable: `initThreeDProjectionInPage` is contractually self-contained (shipped into the page via page.evaluate, no outer-scope references), so the matrix is unreachable from a test without breaking that. Verified at render level; the rationale and method are recorded at the call site. Note the self-verify net cannot catch a regression here — it captures ground truth AFTER this rewrite, so it would compare wrong geometry against wrong geometry. Refs PRINFRA-486
vanceingalls
left a comment
There was a problem hiding this comment.
R1 (COMMENT) — Depth-sign inversion in initThreeDProjectionInPage. One-line negation with a load-bearing comment. Verified the enclosing GL setup matches the comment's premise: gl.clearDepth(1), gl.enable(gl.DEPTH_TEST), gl.depthFunc(gl.LEQUAL) at the top of update(), and gl.frontFace(gl.CW) at group setup. Z_SCALE = 1/100000 at line 734; grep of the file shows the sign is only consumed at the one edited site — no other callers depend on it. No blockers; one non-blocker.
Non-blocker
packages/engine/src/services/threeDProjection.ts:786-816— the comment explicitly documents that the self-verify net cannot catch a re-inversion of this sign (it captures ground truth after this rewrite, so it would comparewrong geometry vs wrong geometry, 65 dB against a truly-broken 18.8 dB render). Since a unit test is architecturally excluded ("shipped into the page viapage.evaluate, no outer-scope references"), the only backstop that survives is a regression comp. Recommend adding a two-plane ±45°preserve-3dcomposition to a regression shard (regression-shardsin.github/workflows/regression.yml), scored against the screenshot arm, so a future re-inversion trips CI at the render-parity level the self-verify net cannot reach. Not required for merge — but the class of regression is now knowingly uncovered.
Scope check
- No adjacent knob was widened. The 3D compile gate still clamps
perspective/preserve-3doff fast capture; projection is only reachable viaHF_FAST_CAPTURE_3D. The PR body's scope note tracks the earlier revision that tried to narrow that gate and reverted after finding six pre-existing projection limitations catalogued on PRINFRA-486. Good discipline. - Sign is scoped: single site of consumption for
Z_SCALE, verified.
Status: MERGEABLE / mergeStateStatus: CLEAN. CI green.
— Via
terencecho
left a comment
There was a problem hiding this comment.
R2 (COMMENT) — Concurring with Via's R1 on the depth-sign fix + independent verification of the flagged residual.
Fix verification
Confirmed the one-line change at packages/engine/src/services/threeDProjection.ts inside initThreeDProjectionInPage's ndc matrix: Z_SCALE → -Z_SCALE. Rationale checks out against the enclosing GL state — clearDepth(1) + depthFunc(LEQUAL) on a context where the y-flip on the row above already flipped handedness (compensated for winding by frontFace(gl.CW) but never for depth). Inverting Z_SCALE flips the sign of the ndc z output so nearer CSS +z now produces smaller ndc z and wins the LEQUAL test. Z_SCALE is consumed only at this one site, so the sign has no other downstream. Fix is correct in-direction; measured deltas (18.8 → 51.2 dB two-plane, single-quad cases unchanged) are consistent with a single-sign inversion at a single site.
Concur on Via's non-blocker (regression-coverage gap)
The load-bearing comment ships an explicit admission: the self-verify net CANNOT catch a regression here, because it captures ground truth after this rewrite and would compare wrong geometry against wrong geometry. So a future re-inversion at this site is knowingly uncovered by the existing self-verify path — the baseline is DOM-post-rewrite, so both arms would carry the same wrong geometry (as measured: 65 dB pass on a truly-broken 18.8 dB output).
The PR body's scope note cites PRINFRA-486, but that ticket enumerates six other projection limitations (group-root paint, layout-animating quads, animated perspective, flat intermediates, self-quad ordering, mid-loop bails) as prerequisites for narrowing the 3D compile gate — it does NOT scope the two-plane ±45° regression comp Via recommended. So the class of regression the comment names as uncovered is currently unscoped.
Recommend opening a dedicated follow-up: a two-plane ±45° preserve-3d composition added to a regression-shards shard, drawElement-vs-screenshot dB-scored, so a re-inversion (or any regression at this site) trips CI at the render-parity level the self-verify net cannot reach. Not blocking — Vance is on record about the architectural constraint (initThreeDProjectionInPage is contractually self-contained via page.evaluate, unit-test-hostile) — but this is the first PR where the comment itself names the gap and it is worth a scoped ticket rather than an implicit note.
CI + merge state
mergeStateStatus: CLEAN,mergeable: MERGEABLE,reviewDecision:empty.- Stack-child reduced matrix as expected:
preview-regression / Preview paritySKIPPED,Player perf / PerfSKIPPED,Preflight (lint + format)SKIPPED on Player perf + preview-regression. Full workflow runs green onregression(Preflight + regression-shards 1-9 + summary), plusplayer-perf,preview-regression, WIP, Mintlify. - Head SHA
564caa5.
— Review by tai (pr-review)
miga-heygen
left a comment
There was a problem hiding this comment.
Approve. Single-character fix (Z_SCALE → -Z_SCALE) with an excellent explanatory comment. CSS/GL depth convention mismatch caused nearest quad to lose depth test. Measured: 18.8 dB → 51.2 dB.
Known limitation (non-blocking, echoing Via): the self-verify net captures ground truth after this rewrite, so a future depth-sign re-inversion would compare wrong geometry against wrong geometry. A two-plane ±45° preserve-3d regression comp at parity level would catch it.
— Miga
… instead of failing (#3172) * fix(engine): bound drawElement frames so a wedged renderer falls back instead of failing A single drawElement frame could kill an entire render. On one comp (caption-editorial-emphasis) drawElementImage returns normally and the renderer then stops draining its task queue: the setTimeout(…, 0) that schedules toDataURL never fires, the capture page.evaluate never settles, and 60s later the stage watchdog fails the whole render. Deterministic on 152.0.7977.30, always the same frame. Root-cause detail in PRINFRA-488. Two gaps, both closed here. 1. Nothing below the 60s stage watchdog bounded a frame. Adds a per-frame deadline (HF_DE_FRAME_TIMEOUT_MS, default 15000, 0 disables) around the WHOLE frame operation, not just the drawElementImage call — the stall surfaces at whichever page round-trip comes next, and was observed at both the capture and the seek's background-image decode. Bounding one call missed it. Deliberately NO per-frame screenshot fallback for this error: once the renderer stops scheduling it is wedged for every subsequent round-trip on that page. Measured — the screenshot fallback blew the same deadline. The frame fails fast instead, so the recovery that actually works can run. 2. That recovery was ineligible. shouldRetryViaPinnedFallback only retried a generic capture failure when the worker count was PINNED by the inversion or the router; a comp that engaged drawElement on the ordinary single-worker path had no whole-render fallback at all. A renderer stall is now retryable on any routing — the failure is a property of drawElement itself, and the retry re-renders on a fresh page via screenshot. Reported as de_fallback_reason "de_renderer_stall" so it is distinguishable from capture_error in telemetry, and counted per-session as deFrameTimeouts. Verified end to end on the repro: previously exit 1 with "stalled: no frame progress for 60000ms"; now the deadline fires, the render retries via screenshot, and RENDER_OK with a valid 1920x1080 / 240-frame / 8.0s MP4. Tests cover the predicate both ways plus the cross-package error match. Engine 1,481 and producer 587 green (audioPadTrim.integration flakes only under full-suite parallel load — passes in isolation, and on clean HEAD it is green too, so it is not from this change). Refs PRINFRA-488 * fix(engine,producer,cli): count drawElement frame timeouts where they happen `session.deFrameTimeouts` was incremented inside `captureFrameCore`'s catch, but the deadline races that function from OUTSIDE it — `withFrameDeadline` is applied by `captureFrameToBuffer`, and `DeFrameTimeoutError` is constructed nowhere else. So the branch was unreachable: on a wedged renderer the counter stayed 0, the engine-side stall log never printed, and `CapturePerfSummary.deFrameTimeouts` reported 0 on every affected render. The end-to-end recovery worked throughout — `isDeRendererStallError` matches by name at the producer — only the observability was dead. Moved the increment and the log into an `onTimeout` hook that fires from the deadline itself, deleted the unreachable branch and the now-unused `isDeFrameTimeoutError`, and threaded the count out to `render_completed.de_frame_timeouts` so the rate is graphable apart from the other `capture_error` fallbacks. A stall always costs a whole-render re-run, which is why it deserves its own series. Also corrected the `fallbackReason` docstring, which still listed only `psnr | blank | oom | capture_error` after this branch added `de_renderer_stall`. `withFrameDeadline` is exported for a two-case fake-timer test: the deadline fires and calls `onTimeout` once, and a resolving inner promise passes its value through with no late timer. The first case fails if the counter moves back inside the work promise.
The base branch was changed.
…streaming `withFrameDeadline` was applied by `captureFrameToBuffer` alone. The disk path (`captureFrame`, reachable under the fast-capture opt-in) and the macOS worker-encode path (`captureFrameToBufferPipelined`) still awaited their round-trip unbounded, so the same wedged renderer PRINFRA-488 describes failed the whole render at the 60 s watchdog there — the exact outcome the deadline exists to replace. The pipelined path does not route through `captureFrameCore` at all; it calls `produceDrawElementFrame` directly, so its produce phase is bounded on its own. All three now share `withDeFrameDeadline`, which also owns the stall counter and log. The pipelined catch rethrows a blown deadline before the per-frame diagnostics: those screenshot and evaluate against the page that just stopped scheduling, so running them would spend the whole budget the deadline saved. Producer side: the disk-capture catch handled only `isDrawElementVerificationError` and rethrew everything else, so even a correctly-raised stall had no whole-render fallback there. It now routes the stall the same way the streaming drain does, under a new `renderer_stall` capture-plan failure kind — `sdr_disk` accepts no other kind, and reusing the verification label would file a fabricated diagnosis in the retry telemetry (nothing was verified; no score exists). Wiring is pinned at the source level in the deadline test, since the entry points need a real Chrome page: each must reference the wrapper, and the pipelined bail must precede its diagnostics call. Unwiring any one of the three fails it.
…over it The comment already admits the self-verify net cannot catch a re-inversion here. Three reviewers asked for the render-parity comp that could. That is not a fixture drop — the harness has no channel for `HF_FAST_CAPTURE_3D`, the golden has to be produced on the Linux/Docker CI platform, and the sibling fast-capture fixture is itself excluded from every shard — so it is scoped in PRINFRA-570 and named here, along with the fact that until it lands this sign has no CI coverage at all.
Stack 2/3 — based on #3231. Review that first; this diff is one line of behaviour plus its rationale.
threeDProjectiondoes run a depth test —gl.enable(gl.DEPTH_TEST)withclearDepth(1)anddepthFunc(LEQUAL). The comparison was inverted: CSS puts +z toward the viewer, GL treats larger ndc z as farther, and the ndc matrix passed CSS z through with a positive scale. So the nearest quad received the largest depth, lost the test, and was occluded by the quad behind it.The y-flip on the row above changes handedness. That was already compensated for winding (
frontFace(gl.CW)) but never for depth — which is why it only surfaced once two quads in one context could be visible simultaneously. A single-quad context has nothing to lose a depth test against.Measurement
Two planes at ±45° in one
preserve-3dcontext, drawElement render vs a screenshot render of the same comp:Single-quad cases are unaffected by the negation, as expected.
Two things reviewers should know
No unit test, and that is structural.
initThreeDProjectionInPageis contractually self-contained — shipped into the page viapage.evaluate, no outer-scope references — so the matrix is unreachable from a test without breaking that contract. Verified at render level instead; the method is recorded at the call site so it can be repeated.The self-verify net cannot catch a regression here. It captures ground-truth frames after this rewrite mutates the DOM, so it compares projected-DOM-via-drawElement against projected-DOM-via-screenshot — both carrying the same geometry. Measured: it passed the broken comp at 65 dB while the true output was 18.8 dB off. Do not treat it as the backstop for 3D changes.
Scope note
This does not open up any renders. The 3D compile gate still clamps
perspective/preserve-3dcomps before capture, so projection is only reachable viaHF_FAST_CAPTURE_3D. An earlier revision of this stack narrowed that gate; it was reverted after review found the gate is load-bearing for six pre-existing projection limitations this fix does not address (group-root paint, layout-animating quads, animated perspective, flat intermediates, self-quad ordering, mid-loop bails). Those are catalogued on PRINFRA-486 as the prerequisites for narrowing.Refs PRINFRA-486
🤖 Generated with Claude Code